Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: fix mockgen version #9127

Merged
merged 13 commits into from
May 25, 2021
Merged

test: fix mockgen version #9127

merged 13 commits into from
May 25, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Apr 16, 2021 •
edited
Loading

Description

  • Use mocgen from version control.
  • update mocgen to the latest version

Closes: #9282

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

amaury1093
amaury1093 reviewed Apr 16, 2021
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using fixed version totally makes sense, but tests fail

@fdymylja
Copy link
Contributor

to fix:

controller.go:266: missing call(s) to *mocks.MockAppModule.DefaultGenesis(is equal to &{0xc00000fcf8}) /home/runner/work/cosmos-sdk/cosmos-sdk/simapp/app_test.go:211
    controller.go:266: missing call(s) to *mocks.MockAppModule.InitGenesis(is equal to {0xc00011e010 {0xc000ebe270 map[0xc000cf1df0:0xc000ebe660 0xc000cf1e00:0xc000ebe720 0xc000cf1e10:0xc000ebed20 0xc000cf1e20:0xc000ebec60 0xc000cf1e30:0xc000ebe7e0 0xc000cf1e40:0xc000ebede0 0xc000cf1e50:0xc000ebeba0 0xc000cf1e60:0xc000ebe8a0 0xc000cf1e70:0xc000ebea20 0xc000cf1e80:0xc000ebe420 0xc000cf1e90:0xc000ebe360 0xc000cf1ea0:0xc000ebe4e0 0xc000cf1eb0:0xc000ebe5a0 0xc000cf1ed0:0xc000ebe960 0xc000cf1ec0:0xc000ebeae0] map[acc:0xc000cf1df0 authz:0xc000cf1eb0 bank:0xc000cf1e00 capability:0xc000cf1ea0 distribution:0xc000cf1e30 evidence:0xc000cf1e90 feegrant:0xc000cf1e80 gov:0xc000cf1e50 mem_capability:0xc000cf1ed0 mint:0xc000cf1e20 params:0xc000cf1e60 slashing:0xc000cf1e40 staking:0xc000cf1e10 transient_params:0xc000cf1ec0 upgrade:0xc000cf1e70] <nil> map[] map[]} {{0 0}  0 {0 0 <nil>} {[] {0 []}} [] [] [] [] [] [] [] [] []}  [] 0xc000cf1d90 [] 0xc000ca5b60 <nil> true false [] <nil> 0xc000e9c6d8}, is equal to &{0xc00000fcf8}, is equal to [123 34 107 101 121 34 58 32 34 118 97 108 117 101 34 125]) /home/runner/work/cosmos-sdk/cosmos-sdk/simapp/app_test.go:212

on simapp/app_test.go line 213 you need to add: mockModule.EXPECT().ConsensusVersion().Times(1).Return(uint64(0))

somehow the autogenerated mocks were edited manually, and not regenerated.

alessio
alessio reviewed Apr 16, 2021
View reviewed changes
Makefile Show resolved Hide resolved
@clevinson clevinson self-assigned this May 5, 2021
@robert-zaremba robert-zaremba linked an issue May 7, 2021 that may be closed by this pull request
Call mockgen with dependency manager / go modules #9282
Closed
@robert-zaremba robert-zaremba added this to the v0.43 milestone May 11, 2021
@robert-zaremba robert-zaremba modified the milestone: v0.43 May 11, 2021
@cyberbono3 cyberbono3 assigned amaury1093 and unassigned clevinson May 18, 2021
@cyberbono3
Copy link
Contributor

@robert-zaremba what issue does this PR close?

@robert-zaremba
Copy link
Collaborator Author

Oh, yes I used wrong number in description. Should be: #9282 (already updated).

robert-zaremba
robert-zaremba commented May 19, 2021
View reviewed changes
types/handler_test.go

mockAnteDecorator2 := mocks.NewMockAnteDecorator(mockCtrl)
mockAnteDecorator1.EXPECT().AnteHandle(gomock.Eq(ctx), gomock.Eq(tx), true, mockAnteDecorator2).Times(1)
mockAnteDecorator2.EXPECT().AnteHandle(gomock.Eq(ctx), gomock.Eq(tx), true, nil).Times(1)
sdk.ChainAnteDecorators(mockAnteDecorator1, mockAnteDecorator2)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is something wrong here: this tests can't pass because in the last case the ante chain is not even called (sdk.ChainAnteDecorators return a decorator, but we still need to call it).

I added a call but we can't correctly check the mockAnteDecorator2 as a expected argument - see the note in the code.

robert-zaremba
robert-zaremba commented May 19, 2021
View reviewed changes
tests/mocks/types_handler.go
ret1, _ := ret[1].(error)
return ret0, ret1
m.ctrl.Call(m, "AnteHandle", ctx, tx, simulate, next)
return next(ctx, tx, simulate)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next is not called in a generated code. So we have to customize the code. Since the test was not functioning, this function never worked as expected.

SOLUTION: I'm removing types_handlers from auto - generated code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fdymylja - you were looking at the mocgen - did you solve it in any other way?

@codecov
Copy link

codecov bot commented May 19, 2021 •
edited
Loading

Codecov Report

Merging #9127 (cf636f3) into master (7c70ca5) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9127   +/-   ##
=======================================
  Coverage   60.40%   60.40%           
=======================================
  Files         589      589           
  Lines       37095    37095           
=======================================
+ Hits        22406    22407    +1     
+ Misses      12712    12711    -1     
  Partials     1977     1977
Impacted Files Coverage Δ
types/handler.go 100.00% <0.00%> (+14.28%) ⬆️

@robert-zaremba
Copy link
Collaborator Author

This is ready for review.

amaury1093
amaury1093 approved these changes May 21, 2021
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

cyberbono3
cyberbono3 approved these changes May 24, 2021
View reviewed changes
Copy link
Contributor

@cyberbono3 cyberbono3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label May 25, 2021
@robert-zaremba robert-zaremba changed the title setup: update mockgen test: fix mockgen version May 25, 2021
@robert-zaremba robert-zaremba removed the A:automerge Automatically merge PR once all prerequisites pass. label May 25, 2021
@robert-zaremba robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label May 25, 2021
@mergify mergify bot merged commit 8c1fe4b into master May 25, 2021
@mergify mergify bot deleted the robert/update-mockgen branch May 25, 2021 09:19
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@robert-zaremba @tac0turtle
test: fix mockgen version (cosmos#9127)
280002c 
* setup: update mockgen

* Add expect ConsensusVersion in app_test

* fix ChainAnteDecorators tests

* remove types/handler.go from autogenerating mocks

* adding a note

* adding note

Co-authored-by: Alessio Treglia <alessio@tendermint.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleem1314 aleem1314 aleem1314 approved these changes

@cyberbono3 cyberbono3 cyberbono3 approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@amaury1093 amaury1093 amaury1093 approved these changes

@jgimeno jgimeno Awaiting requested review from jgimeno

@aaronc aaronc Awaiting requested review from aaronc

@anilcse anilcse Awaiting requested review from anilcse

@blushi blushi Awaiting requested review from blushi

@fdymylja fdymylja Awaiting requested review from fdymylja

@technicallyty technicallyty Awaiting requested review from technicallyty

@alessio alessio Awaiting requested review from alessio

Assignees

@amaury1093 amaury1093

@cyberbono3 cyberbono3

Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
v0.43
Development

Successfully merging this pull request may close these issues.

Call mockgen with dependency manager / go modules
10 participants
@robert-zaremba @fdymylja @cyberbono3 @alessio @amaury1093 @alexanderbez @aleem1314 @aaronc @clevinson @tac0turtle